allow using lazy loadable renderers in angular #2446
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
dsl400
changed the title
This reverts commit 3f5b96f.
|
Hi @dsl400 , |
|
I was hoping to migrate our existing code by loading some components as custom renderers until we have time to create schema for all our existing forms |
|
I don't think this PR is necessary. Instead of within JSON Forms you could await the renderers outside of JSON Forms and only render JSON Forms once they are resolved. |
Can you provide some technical arguments about why ? My argument is: It has no impact on existing functionality |
|
Hi @dsl400, I’m not sure this needs to be built into JSON Forms. Since you're writing custom renderers anyway, you can wrap them to handle async loading yourself. This keeps the framework cleaner and avoids adding complexity. |
|
@sdirix @sdirix @lucas-koehler I would like to get more information on how to test the scenario you were concerned about |
|
I discussed this again with @lucas-koehler and we came to the conclusion that this feature could be useful for multiple adopters, so we would accept a polished contribution. |
| bestComponent = renderer.renderer; | ||
| if (renderer.renderer instanceof Promise) { | ||
| renderer.renderer.then((resolvedRenderer) => { | ||
| bestComponent = resolvedRenderer; |
There was a problem hiding this comment.
This assignment points to a code smell:
- Either it has no effect, because the
updatecode is already completed, - Or the promise is already resolved and the Javascript engine decides to also run this "async" snippet immediately. However in that case the component will be rendered twice, once by this "async" snipped and once by the
updatecode.
| const componentFactory = | ||
| this.componentFactoryResolver.resolveComponentFactory(bestComponent); | ||
| this.renderComponent(componentFactory, uischema, schema, props); |
There was a problem hiding this comment.
For async components, this will lead to the UnknownRenderer being rendered first which is misleading and might look like an error to the user. Instead we should render a LoadingRenderer or something similar.
| @@ -126,11 +127,32 @@ export class JsonFormsOutlet | |||
| renderer !== undefined && | |||
| renderer.tester(uischema, schema, testerContext) !== -1 | |||
| ) { | |||
| bestComponent = renderer.renderer; | |||
| if (renderer.renderer instanceof Promise) { | |||
| renderer.renderer.then((resolvedRenderer) => { | |||
There was a problem hiding this comment.
There can be a race condition here:
- The outlet is invoked, however the renderer (Renderer A) is a promise and needs some time to complete
- There is an update in the mean time, another renderer (Renderer B) is required and it is awaited too
- The Renderer B resolves first and renders
- Now finally Renderer A resolves and will be used for rendering, although Renderer B is the one which should be used.
| @@ -126,11 +127,32 @@ export class JsonFormsOutlet | |||
| renderer !== undefined && | |||
| renderer.tester(uischema, schema, testerContext) !== -1 | |||
| ) { | |||
| bestComponent = renderer.renderer; | |||
| if (renderer.renderer instanceof Promise) { | |||
| renderer.renderer.then((resolvedRenderer) => { | |||
There was a problem hiding this comment.
We could also think about storing the resolved component somewhere, maybe even in the registry entry. This would be useful so that once a renderer is resolved, we avoid the whole "LoadingSpinner" code path the next time it is invoked.
|
@sdirix Thank you very much for reconsidering this PR, I will come back with updates |
proposal to add possibility to use lazy loadable renderers